Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Testing with Nodeunit (through the command-line) #63

Closed
wants to merge 1 commit into from
Closed

Conversation

bigkevmcd
Copy link
Contributor

I've tried to stick to the same format as the Jasmine testing page (using the same example, to make it fairly easy to do a side-by-side comparison).

Any suggestions would be welcome!

@sukima
Copy link
Contributor

sukima commented Mar 2, 2013

This seems like a reasonable contribution. Is there any reason for this to not be merged? (Speaking to the Cookbook team in general). I think this should be merged if no one speaks up in the next week or so.

@dbrady
Copy link
Member

dbrady commented Mar 3, 2013

Yeah, I noticed we had a bunch of open reqs that weren't getting any love. So yesterday I went through all the open requests and merged all but this one. I love the ambition of this submission and I definitely want this topic brought in. I didn't merge it right away for a couple reasons, but first I want to apologize to @bigkevmcd for not getting this looked at in a timely manner. Thank you for this submission! We just suck, is all. :-)

I didn't merge this one yesterday because this is a pretty good example of an "overwhelming submission". :-) I'm still trying to wrap my head around it, and I'd like to at least test it before merging it. Reading through it, I realize that I'm going to have to learn how to install Nodeunit before I can do that. It's not @bigkevmcd's job to provide setup instructions, so I'm not faulting him. And it's probably even very simple to do. I just haven't had time to make brainspace to tackle this.

I do have one concern with the format of this submission, though; it is the first recipe to use screenshots instead of text, and it's not clear to me why. If this were a graphics technique, a styling trick, or some other visually-oriented recipe, it would make perfect sense. But these are screenshots of text. Accessibility reasons aside, I can't verify that the output is correct with a computer. The irony of this in a chapter about automated testing does not escape me. :-) (I do see that the text is colorized, and I'm guessing the author's intent was to capture that. But is this more important than the content of the text itself? I'm asking, not challenging. I would say no, but I'm happy to hear differing opinions.)

Anyway, short version is, I'd like myself or somebody else to at least try the recipe, and I'd love to see about working it around to not depend on screenshot images.

Thoughts?

@dbrady
Copy link
Member

dbrady commented Mar 3, 2013

Two other things: one, I just re-read the submission more carefully and saw that there IS a section on installing nodeunit. This is the sort of thing that happens with overwhelming submissions: epic post + ADD = reading comprehension fail. :-)

Secondly, I forgot to thank @bigkevmcd for this submission. It is awesome, and I definitely want to get it into the cookbook. Thank you!

@sukima
Copy link
Contributor

sukima commented Mar 3, 2013

I'll verify this and try it on my command line.. I'll also grab pasted output. @bigkevmcd, any objections if I convert screen images to text? Give me a day or so. Also @bigkevmcd if I make a new pull request you'll loose your commit history. If you want to keep that can you break up the pull request into multiple commits? Otherwise I'll make my own pull request to supersede this one.

@sukima sukima mentioned this pull request Mar 3, 2013
@sukima
Copy link
Contributor

sukima commented Mar 3, 2013

Superseded by pull request #73. Closing this one.

@sukima sukima closed this Mar 3, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants